Skip to content

Conversation

@aslafy-z
Copy link
Contributor

@aslafy-z aslafy-z commented Apr 6, 2023

  • Pass options.retryBackoff to new transport.

Fixes #1692

Copy link
Contributor

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonjohnsonjr aside from the 429 change, do you see a reason we shouldn't retry with the custom semantics when we're also injecting a custom transport?

@jonjohnsonjr
Copy link
Collaborator

I had these be intentionally separate because if you want to control the transport retry behavior, you should be using remote.WithTransport.

@aslafy-z
Copy link
Contributor Author

aslafy-z commented Apr 7, 2023

@jonjohnsonjr if so, I don't get what are all these options for..

@aslafy-z aslafy-z changed the title fix(options): pass retryBackoff and retryPredicate options to transport feat(remote): pass retryBackoff and retryPredicate options to transport Apr 7, 2023
@github-actions
Copy link

github-actions bot commented Jul 7, 2023

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@aslafy-z
Copy link
Contributor Author

aslafy-z commented Jul 7, 2023

This is still desired. @jonjohnsonjr is this still something you don't want for go-containerregistry?

@block-danm
Copy link

@jonjohnsonjr Could you expand on

if you want to control the transport retry behavior, you should be using remote.WithTransport.

please?

There are three settings that control retry behavior - status codes, predicate, and backoff - and currently all three are set in different ways:

  • the status codes that trigger a retry attempt are set from o.retryStatusCodes, so they account for any settings provided with config.WithRetryStatusCodes();
  • the retry predicate is always set to the defaultRetryPredicate defined in options.go;
  • the backoff isn't explicitly set in makeOptions(), so it falls back to the default defined in retry.go.

This inconsistency is quite unexpected and confusing, and this PR would immediately resolve that confusion.

I am also very curious about @aslafy-z 's question - if config.WithRetryBackoff() does not affect the retry backoff used by the actual transport, what is it for?

@aslafy-z
Copy link
Contributor Author

aslafy-z commented Nov 9, 2023

@imjasonh @jonjohnsonjr Can you please reconsider this PR? Thank you

@aslafy-z aslafy-z requested a review from imjasonh November 9, 2023 16:06
@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.88%. Comparing base (8b3c303) to head (f6da36f).
Report is 5 commits behind head on main.

Current head f6da36f differs from pull request most recent head 10c7e61

Please upload reports for the commit 10c7e61 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1628      +/-   ##
==========================================
+ Coverage   71.67%   71.88%   +0.21%     
==========================================
  Files         123      122       -1     
  Lines        9935     9832     -103     
==========================================
- Hits         7121     7068      -53     
+ Misses       2115     2081      -34     
+ Partials      699      683      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@aslafy-z
Copy link
Contributor Author

This PR is still desired.

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@aslafy-z aslafy-z force-pushed the fix/transport-retry-backoff-option branch 2 times, most recently from d40aa02 to 10c7e61 Compare May 25, 2024 17:41
@aslafy-z
Copy link
Contributor Author

I just rebased and fixed conflicts to this PR. Now that #1635 is merged, can we merge that one? Thank you

@hayk99
Copy link

hayk99 commented Jul 1, 2024

is this going to be merged soon?

@aslafy-z
Copy link
Contributor Author

aslafy-z commented Jul 1, 2024

I would love to see this merged..

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@aslafy-z aslafy-z force-pushed the fix/transport-retry-backoff-option branch from 10c7e61 to a138e4d Compare September 30, 2024 07:31
@aslafy-z
Copy link
Contributor Author

This is not stale.

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@aslafy-z
Copy link
Contributor Author

Any news please? Thank you

@markusthoemmes
Copy link
Contributor

Ack on this PR. @jonjohnsonjr or @imjasonh it feels like the current implementation is a bug in that the respective retry mechanisms aren't consistently applied. Or am I missing something?

@aslafy-z aslafy-z force-pushed the fix/transport-retry-backoff-option branch from 4130229 to 7d5480e Compare October 28, 2025 11:46
@aslafy-z aslafy-z changed the title feat(remote): pass retryBackoff and retryPredicate options to transport feat(remote): pass retryBackoff option to transport Oct 28, 2025
@aslafy-z aslafy-z force-pushed the fix/transport-retry-backoff-option branch from 7d5480e to c01c5e4 Compare October 28, 2025 11:47
@aslafy-z
Copy link
Contributor Author

aslafy-z commented Oct 28, 2025

@Subserial I've seen you've approved #2135, can this one can also go through?

@Subserial Subserial merged commit aab7c77 into google:main Oct 28, 2025
16 checks passed
@aslafy-z aslafy-z deleted the fix/transport-retry-backoff-option branch October 28, 2025 20:36
@aslafy-z
Copy link
Contributor Author

Thank you @Subserial!

@aslafy-z
Copy link
Contributor Author

@Subserial Any chance a release gets cut anytime soon?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ggcr: Pass retryBackoff to transport.NewRetry in the remote pkg

8 participants